Fix eager-load key type coercion#274
Open
jbeers wants to merge 1 commit into
Open
Conversation
elpete
approved these changes
May 13, 2026
There was a problem hiding this comment.
Pull request overview
This PR fixes eager-loading key handling so numeric IDs remain numeric through key collection/deduplication, preventing QueryBuilder from binding them as string/varchar types during relationship eager loads.
Changes:
- Update eager-load key collection/deduplication to preserve original key value types while still deduping composite keys.
- Adjust
BelongsTo.getEagerEntityKeys()andBaseRelationship.getKeys()to use a serialized representation only as a lookup key. - Add regression tests asserting integer binding types for eager-loaded
belongsToandbelongsToManyrelationships.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
models/Relationships/BaseRelationship.cfc |
Changes eager-load key collection to preserve original key array values and dedupe via serialized lookup keys. |
models/Relationships/BelongsTo.cfc |
Updates eager-load entity key extraction to retain original key arrays and dedupe via serialized lookup keys. |
tests/specs/integration/BaseEntity/Relationships/EagerLoadingSpec.cfc |
Adds regression coverage to ensure eager-load query bindings remain numeric (integer) rather than varchar. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return isStruct( binding ) && ( binding.keyExists( "cfsqltype" ) || binding.keyExists( "sqltype" ) ); | ||
| } ) | ||
| .map( function( binding ) { | ||
| return lCase( binding.cfsqltype ?: binding.sqltype ); |
Comment on lines
+362
to
+374
| var uniqueKeys = arguments.entities.reduce( function( acc, entity ) { | ||
| var keyValues = []; | ||
| for ( var key in keys ) { | ||
| var value = structKeyExists( entity, "isQuickEntity" ) ? entity.retrieveAttribute( key ) : entity[ key ]; | ||
| if ( entityIsNullValue( baseEntity, key, value ) ) { | ||
| return acc; | ||
| } | ||
| acc.append( keyValues.toList() ); | ||
| return acc; | ||
| }, [] ) | ||
| ).map( function( key ) { | ||
| return key.listToArray(); | ||
| } ); | ||
| keyValues.append( value ); | ||
| } | ||
|
|
||
| acc[ serializeJSON( keyValues ) ] = keyValues; | ||
| return acc; | ||
| }, {} ); |
Collaborator
|
@copilot Can you verify if the values used in the bindings are expanded to full struct bindings before being serialized as JSON to check for duplicate keys in the code path this PR is covering? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This fixes eager-loading paths that were coercing numeric key values to strings before QB built the relationship query bindings.
The bug came from using
toList()/listToArray()as both:That round-trip converted numeric IDs like
1into string values like"1", which then caused QB to bind them asvarcharinstead ofinteger.Changes
BaseRelationship.getKeys()BelongsTo.getEagerEntityKeys()belongsToandbelongsToManyValidation
server-lucee@6.jsonbox testbox run bundles=tests/specs/integration/BaseEntity/Relationships/EagerLoadingSpec.cfc